Maintable leaves the Swing year and comes back in a new JavaFX dress #3591

tobiasdiez · 2017-12-30T23:35:28Z

2017:

2018:

There are still many many things missing for a fully working version but I thought I give you an early preview. Feedback welcome.
Happy new year everybody!

Fixes #3356, fixes #3532 and fixes #3112.

Known bugs: (ticked = resolved)

Changes to entries don't mark database as changed.
Changes to entries are not reflected in main table.
Cleanup does not work.
Ctrl + A in entry editor selects all entries in table. [not reproducable]
Escape does not close entry editor.
Column width is not correct if the option "Resize to fit window width" is activated
Select multiple entries by holding mouse button: code to fix

Status of features:

Sorting (Shift + click for multi-column sort)
Filtering by search and groups
Context menus for the icons
Right-click menu
Setting special fields like reading status / priority / ... through the main table
Marked entries are not highlighted
Drag & drop to and from the table
Float mode does not work
Color cells in table depending on whether the field is optional/required

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

…Table

Siedlerchr · 2017-12-31T08:42:34Z

Cool! Could you point me to new implementation? In your large commit I see only a lot of Icon changes

tobiasdiez · 2017-12-31T12:00:11Z

Oh, sorry - I forgot to commit all files. The main changes are in the gui.maintable package.

…Table

Siedlerchr · 2018-01-02T10:08:30Z

While you are working on this, you might also take a look at #3356
We could use the Latex2Html converter and then convert that HTML formatting to the TextFlow which was done in the search component tooltips

tobiasdiez · 2018-01-02T10:43:00Z

Yes, this issue is already fixed merely by switching to JavaFX - the unicode is correctly displayed (see above the second entry in the table, the first screenshot shows the boxes while the correct string is shown in the second screenshot).

Siedlerchr · 2018-01-02T10:51:52Z

src/main/java/org/jabref/gui/maintable/ColumnFactory.java

+
+import org.fxmisc.easybind.EasyBind;
+
+class ColumnFactory {


I am not that happy with this generic name here... as this shadows the javafx classes
Maybe MaintableColumnFactory? Same as with the other CellFactory

Siedlerchr · 2018-01-02T10:53:07Z

src/main/java/org/jabref/gui/maintable/ColumnFactory.java

+        }
+
+        // Add columns for other file types
+        for (String column : preferences.getExtraFileColumns()) {


Maybe also addAll possible?=

Siedlerchr · 2018-01-02T10:54:17Z

src/main/java/org/jabref/gui/maintable/StringTableColumn.java

+
+        Optional<String> content = Optional.empty();
+        for (String field : bibtexFields) {
+            content = entry.getResolvedFieldOrAlias(field, database.orElse(null));


Maybe stream with firstOrNull? or anyMatch?

not possible because isNameColumn has to be set.

Siedlerchr · 2018-01-02T10:58:23Z

src/main/java/org/jabref/migrations/FileLinksUpgradeWarning.java

-            List<String> fields = tabList.getTabFields(i);
-            for (String field : fields) {
+        for (Map.Entry<String, List<String>> tab : Globals.prefs.getEntryEditorTabList().entrySet()) {
+            for (String field : tab.getValue()) {


tab.stream.anymatch( entry -> entry.getKey.equals(FieldName.File)

Not sure that this is more readable, especially since the if statement still has to be there.

lenhard · 2018-01-02T14:54:40Z

One more step in the right direction. This PR has the potential to fix a lot of display bugs and we should be able to get rid of the annoying and unmaintained glazed lists library.

Nevertheless, I need to add a word of caution: We cannot allow the same situation as for the entry editor again. We've spent almost half a year in un-releaseable limbo because of that. The replacement for the main table needs to be absolutely bullet proof. However, the amount of lines of code in this PR is too much for a meaningful review, so we need A LOT of manual testing by basically all developers.

I've played around a bit and I like the new layout of the maintable. What's a little strange is that every second row is highlighted differently. This should change for the final version.

EDIT: Things that should be fixed with this PR ;) https://github.com/JabRef/jabref/issues?q=is%3Aissue+is%3Aopen+label%3Amaintable

tobiasdiez · 2018-01-02T16:10:44Z

@Siedlerchr Thanks for the feedback. I'll improve the code accordingly.

@lenhard I completely agree! We should find a way to have a realeasable version while still work on the migration to JavaFX. The main problem is that the code is tightly coupled to the maintable (i.e. there is no abstraction layer before the BasePanel/MainTable that used in the other Swing dialogs). This has the potential upset that every single dialog may now trigger a bug (for example because it runs some code not on the JavaFX thread). So, even through I will try to thoroughly test the new code before marking it as "ready-for-review", there might still be countless bugs left which we need to find (and fix) in teamwork. Moreover, for the same reason, I think we also need to release a beta again.

That being said, the immediate question is: how much functionality should I try to migrate in this PR and what should follow as new PRs? As you said, the size of the changes makes the PR already unreviewable. But I would really like to have your feedback about some design questions. Especially since our aim should be not just to improve the gui but also decouple the code and make it more reusable.

Currently the new maintable is in a state where the very basic functionality works and the code is decoupled enough to actually write unit gui tests for it. There are still some bugs/feature that definitely need to be fixed before a users touches it (e.g. restore column width, context menus for the icons, a few display bugs). Then there are a few features that are not essential and rather isolated (e.g. "marking of entries"). These are probably good candidates for follow-up PRs. And finally there are features that I don't really see any benefit in reimplementing (e.g. coloring of table cells when the field is optional/required) or that are not really possible to implement with JavaFX (e.g. "float search"). Finally, the possible bug fixes @lenhard mentions should also follow as new PRs.

Any thoughts on this? (Sorry for the long text).

Siedlerchr · 2018-01-02T16:22:17Z

I think we could have a separate main Table Beta Test-Release in an additional branch highly experimental and let some users test this functionality who are interested. Especially we would need some Mac/Linux guys

koppor · 2018-01-02T22:11:33Z

We can create a javafx branch, where we can integrate not ready features, but completed work packages (such as a basic entry table in JavaFX)

The master branch must be release-ready at all times. Release-ready means: all features of the old versions should be working (or the dev team decided to remove/disable a feature, such as the auto completion).

…Table

tobiasdiez · 2018-01-09T09:41:46Z

So, I used this version now in production during the past few days and fixed a lot of bugs. Although not all features are yet migrated (see list above), I mark this PR now as ready for review. All further changes are coming as extra PRs onto this branch.

I would suggest that you try this version out and if nothing serious pops up, I would create a post in the forum advertising this PR and ask users about their opinion (something like a very early access program instead of several beta releases). What do you think?

lenhard · 2018-01-09T09:53:02Z

I would be really cool if you could provide a table that lists all features related to the maintable that were present in the Swing version, e.g. sorting options, right click options, entry drop, column sorting, and could indicate if these features are working in the new version. Then we would have an overview of what features actually exist in the main table and we could have a meaningful discussion about which features we want to drop.

Personally, I don't really have an overview of all the things that the maintable can do. Could you perhaps provide this overview? I guess also @koppor know a thousand maintable features that I do not know.

koppor · 2018-01-09T10:37:45Z

I changed the base branch to "maintable-beta". I would propose to proceed as follows:

@JabRef/developers review
Work-in review comments
Merge into maintable-beta.
Ask users to feedback to maintable-beta
Add new PRs on maintable-beta

Why?

I think, PRs should be short and be closed fast. Not left open as the ones of @koppor. So, we can move on quickly. And we can also link to comments of this PR if necessary.
The discussions here are mixed on the implementation details and overall issues. I think, we should focus on implementation details 😇

In case we need a overview-pr, we can create a PR from maintable-beta to master and describe the overall status there.

koppor · 2018-01-09T10:52:20Z

I am very used to the floating search. See screenshots at #3618.

tobiasdiez · 2018-01-09T11:10:22Z

@lenhard I updated the first post to also include the features that are already migrated (it is only the very basic functionality). Right now we do not need to decide what features should be dropped since there is still a lot fundamental functionality to migrate (but we should speak about it in the next devcall). I just don't want this PR to explode even more and get feedback about some of the other refactorings / changes. Thus the early ready-for-review.

@koppor Sounds like a good plan.

* upstream/master: (40 commits) CleanupStep.MOVE_PDF is not a CleanupPreset anymore Add link and fix casing Explicitly set jacoco version (#3617) Update gradle from 4.4 to 4.4.1 Update bcprov-jdk15on from 1.58 -> 1.59 Fix NPE when changing entries between databases Add exporter desc to enum analog to import (#3606) Create 0001-use-crowdin-for-translations.md [ci skip] Get more Open Source Helpers (#3601) Update dependencies (#3602) Lookup filetypes in enum set to prevent NPE due to uninitialized expo… (#3597) Make it possible to disable autocompletion in the search bar (#3600) New translations JabRef_en.properties (Chinese Simplified) New translations JabRef_en.properties (French) New translations JabRef_en.properties (German) New translations JabRef_en.properties (Indonesian) New translations JabRef_en.properties (Italian) New translations JabRef_en.properties (Japanese) New translations JabRef_en.properties (Russian) New translations JabRef_en.properties (Spanish) ...

…or and it is not used anylonger

Siedlerchr · 2018-01-09T16:59:13Z

I fixed the Close Entry Editor with Escape bug

…avafxTable * 'javafxTable' of https://github.com/JabRef/jabref: Update CHANGELOG.md

…Table

…avafxTable

koppor · 2018-01-18T07:14:26Z

@lenhard @Siedlerchr Is this architecturally OK? Then, I would propose to merge it into maintable-beta to reduce the size of PRs.

Note: The "real" PR of getting this into JabRef is #3621 - there, code updates are only made as PRs to that branch.

lenhard

@koppor That's very hard to tell with a PR that big. It would be extremely helpful if the travis build was working for the PR, then the tests would report obvious violations.

I've had a look at the model and logic classes and could not spot a direct violation. There is one suggestion in a TODO, though. Maybe this is fixable? Otherwise the PR could move on imho.

lenhard · 2018-01-18T09:14:55Z

src/main/java/org/jabref/model/entry/BibtexSingleField.java

@@ -8,7 +8,8 @@
 */
 public class BibtexSingleField {

-    public static final int DEFAULT_FIELD_LENGTH = 100;
+    // TODO: This constant should be moved to the gui package, probably to MainTableColumnFactory
+    public static final double DEFAULT_FIELD_LENGTH = 100;


Would it be possible to move this as suggested in the comment?

This is not possible at the moment since the field is still used in other places in model. The new main table is free from this dependency but the import and search dialog still rely on it as the have before.

Ok, no problem. Then just leave as it is.

* First prototype * Make old actions list an enum * Make it work! * Implement feedback

* Show special field icons properly in main table * Make icons functional * Fix NPE * Handle file icon click

tobiasdiez added 4 commits December 28, 2017 16:14

Start converting

2ebc439

Merge branch 'master' of https://github.com/JabRef/jabref into javafx…

497fcdf

…Table

Refactor icons (less awt)

f70ea5d

More icon related changes

952c032

tobiasdiez added 2 commits December 31, 2017 12:48

First working prototype

41eeb3c

Add new classes

b233517

tobiasdiez added 4 commits January 1, 2018 15:21

Hold reference of divider pane position binding

c2b70c3

Merge remote-tracking branch 'refs/remotes/JabRef/master' into javafx…

3b07e3c

…Table

Fix divider location storage

452147d

Flat design for tab header

09b000b

Siedlerchr reviewed Jan 2, 2018

View reviewed changes

Add dummy context menu to row

d7d00a8

tobiasdiez added 6 commits January 3, 2018 20:28

Merge branch 'master' of https://github.com/JabRef/jabref into javafx…

501b6cc

…Table

Improve hover and selected row style

7f6c924

Further improve hover and selected row style

dc41f29

Restore column width (except when "fit to width" option is enabled)

38f0d15

Update entry editor and preview

7e21bc4

Filter by groups and search

667f1c4

tobiasdiez mentioned this pull request Jan 8, 2018

Editor gets stuck and refuses to show the fields #3616

Closed

1 task

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 9, 2018

koppor changed the base branch from master to maintable-beta January 9, 2018 10:31

tobiasdiez mentioned this pull request Jan 9, 2018

Entry preview and editor blank after #3608

Closed

1 task

koppor added the maintable label Jan 9, 2018

tobiasdiez mentioned this pull request Jan 9, 2018

[WIP] Main table JavaFX migration #3621

Merged

35 tasks

Siedlerchr and others added 3 commits January 9, 2018 17:34

Remove clear Search keybinding as this overlaps with Close entry edit…

2c7b3a7

…or and it is not used anylonger

Merge branch 'maintable-beta' into javafxTable

01f0af2

Siedlerchr and others added 6 commits January 9, 2018 18:11

wrap showandEdit in javafx thread to make new entry actions work

35d32c5

Merge branch 'javafxTable' of https://github.com/JabRef/jabref into j…

1c41425

…avafxTable * 'javafxTable' of https://github.com/JabRef/jabref: Update CHANGELOG.md

Improve automatic sizing of columns

e3515e9

Implement feedback

bd2c1e3

Merge branch 'master' of https://github.com/JabRef/jabref into javafx…

b0af742

…Table

Merge branch 'javafxTable' of https://github.com/JabRef/jabref into j…

8c68b86

…avafxTable

lenhard reviewed Jan 18, 2018

View reviewed changes

tobiasdiez mentioned this pull request Jan 24, 2018

space character causes cursor to jump to start of line #3471

Closed

tobiasdiez added 3 commits January 29, 2018 12:59

Bring back the context menu (#3666)

8980713

* First prototype * Make old actions list an enum * Make it work! * Implement feedback

Reimplement special field and file columns (#3647)

fbc5707

* Show special field icons properly in main table * Make icons functional * Fix NPE * Handle file icon click

Merge branch 'maintable-beta' into javafxTable

3a2ee4e

tobiasdiez merged commit e5bf83c into maintable-beta Jan 29, 2018

tobiasdiez deleted the javafxTable branch January 29, 2018 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintable leaves the Swing year and comes back in a new JavaFX dress #3591

Maintable leaves the Swing year and comes back in a new JavaFX dress #3591

tobiasdiez commented Dec 30, 2017 •

edited

Loading

Siedlerchr commented Dec 31, 2017

tobiasdiez commented Dec 31, 2017

Siedlerchr commented Jan 2, 2018

tobiasdiez commented Jan 2, 2018

Siedlerchr Jan 2, 2018

Siedlerchr Jan 2, 2018

Siedlerchr Jan 2, 2018

tobiasdiez Jan 9, 2018

Siedlerchr Jan 2, 2018

tobiasdiez Jan 9, 2018

lenhard commented Jan 2, 2018 •

edited

Loading

tobiasdiez commented Jan 2, 2018 •

edited

Loading

Siedlerchr commented Jan 2, 2018

koppor commented Jan 2, 2018

tobiasdiez commented Jan 9, 2018

lenhard commented Jan 9, 2018

koppor commented Jan 9, 2018

koppor commented Jan 9, 2018

tobiasdiez commented Jan 9, 2018

Siedlerchr commented Jan 9, 2018

koppor commented Jan 18, 2018

lenhard left a comment

lenhard Jan 18, 2018

tobiasdiez Jan 18, 2018

lenhard Jan 19, 2018

Maintable leaves the Swing year and comes back in a new JavaFX dress #3591

Maintable leaves the Swing year and comes back in a new JavaFX dress #3591

Conversation

tobiasdiez commented Dec 30, 2017 • edited Loading

Siedlerchr commented Dec 31, 2017

tobiasdiez commented Dec 31, 2017

Siedlerchr commented Jan 2, 2018

tobiasdiez commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard commented Jan 2, 2018 • edited Loading

tobiasdiez commented Jan 2, 2018 • edited Loading

Siedlerchr commented Jan 2, 2018

koppor commented Jan 2, 2018

tobiasdiez commented Jan 9, 2018

lenhard commented Jan 9, 2018

koppor commented Jan 9, 2018

koppor commented Jan 9, 2018

tobiasdiez commented Jan 9, 2018

Siedlerchr commented Jan 9, 2018

koppor commented Jan 18, 2018

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Dec 30, 2017 •

edited

Loading

lenhard commented Jan 2, 2018 •

edited

Loading

tobiasdiez commented Jan 2, 2018 •

edited

Loading